-
-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
teamviewer: fix #96633, #44307 and #97148 + 15.15.5 -> 15.18.5 -> 15.22.3 #140076
teamviewer: fix #96633, #44307 and #97148 + 15.15.5 -> 15.18.5 -> 15.22.3 #140076
Conversation
Fix teamviewer's breakage post 15.5.3 -> 15.15.5. Teamviewer client was no longer able to connect to its backing server as it now uses dbus to do so. Following changes were required: - add missing dbus and polkit service/policy files to package. - add missing dbus lib to `LD_LIBRARY_PATH`. Changes to the nixos module as a separate changeset.
Add teamviewer package as a dbus package now that the client / server communication depends on dbus.
Move to a forefront launch of the daemon. Doing so allowed us to move the service from forking to simple to avoid the missing pid error log. Also: - Make the dbus dependency explicit.
qt5 is currently at |
this patch reduces duplication and add coreutils to fix #97148 diff --git a/pkgs/applications/networking/remote/teamviewer/default.nix b/pkgs/applications/networking/remote/teamviewer/default.nix
index 60a134e2435..9337e67e191 100644
--- a/pkgs/applications/networking/remote/teamviewer/default.nix
+++ b/pkgs/applications/networking/remote/teamviewer/default.nix
@@ -1,6 +1,6 @@
{ mkDerivation, lib, fetchurl, autoPatchelfHook, makeWrapper, xdg-utils, dbus
, qtbase, qtwebkit, qtwebengine, qtx11extras, qtquickcontrols, getconf, glibc
-, libXrandr, libX11, libXext, libXdamage, libXtst, libSM, libXfixes
+, libXrandr, libX11, libXext, libXdamage, libXtst, libSM, libXfixes, coreutils
, wrapQtAppsHook
}:
@@ -66,22 +66,26 @@ mkDerivation rec {
ln -s $out/share/teamviewer/tv_bin/desktop/teamviewer_$i.png $out/share/icons/hicolor/$size/apps/TeamViewer.png
done;
+ # all the seds and substituteInPlaces should be moved to postPatch
sed -i "s,/opt/teamviewer,$out/share/teamviewer,g" $out/share/teamviewer/tv_bin/desktop/com.teamviewer.*.desktop
substituteInPlace $out/share/teamviewer/tv_bin/script/tvw_aux \
--replace '/lib64/ld-linux-x86-64.so.2' '${glibc.out}/lib/ld-linux-x86-64.so.2'
substituteInPlace $out/share/teamviewer/tv_bin/script/tvw_config \
--replace '/var/run/' '/run/'
-
- wrapProgram $out/share/teamviewer/tv_bin/script/teamviewer --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ libXrandr libX11 dbus ]}"
- wrapProgram $out/share/teamviewer/tv_bin/teamviewerd \
- --prefix PATH : "${lib.makeBinPath [ getconf ]}" \
- --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ libXrandr libX11 dbus ]}"
- wrapProgram $out/share/teamviewer/tv_bin/TeamViewer --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ libXrandr libX11 dbus ]}"
- wrapProgram $out/share/teamviewer/tv_bin/TeamViewer_Desktop --prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [libXrandr libX11 libXext libXdamage libXtst libSM libXfixes dbus ]}"
-
- wrapQtApp $out/share/teamviewer/tv_bin/script/teamviewer
- wrapQtApp $out/bin/teamviewer
+ '';
+
+ makeWrapperArgs = [
+ "--prefix PATH : ${lib.makeBinPath [ getconf coreutils ]}"
+ "--prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ libXrandr libX11 libXext libXdamage libXtst libSM libXfixes dbus ]}"
+ ];
+
+ postFixup = ''
+ wrapProgram $out/share/teamviewer/tv_bin/teamviewerd ''${makeWrapperArgs[@]}
+ # tv_bin/script/teamviewer runs tvw_main which runs tv_bin/TeamViewer
+ wrapProgram $out/share/teamviewer/tv_bin/script/teamviewer ''${makeWrapperArgs[@]} ''${qtWrapperArgs[@]}
+ wrapProgram $out/share/teamviewer/tv_bin/teamviewer-config ''${makeWrapperArgs[@]} ''${qtWrapperArgs[@]}
+ wrapProgram $out/share/teamviewer/tv_bin/TeamViewer_Desktop ''${makeWrapperArgs[@]} ''${qtWrapperArgs[@]}
'';
dontStrip = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the contributing guide when naming your commits/PRs.
485b345
to
355c0bd
Compare
Done (PR name). Not sure what's wrong with changesets however. |
Upgrading to the last version still using qt5.14. Later version will be using qt5.15 which is not in 21.05 stable branch. This fixes us the crash observed in 15.15.5 when stopping the service.
355c0bd
to
db889eb
Compare
This centralizes `PATH` and `LD_LIBRARY_PATH`, avoid multiple layers of wrappers. Refactor as suggested by @Artturin in PR provided patch: <NixOS#140076 (comment)>.
Simply add `coreutils` as a runtime dependency which will prevent teamviewer from using incomplete busybox implementation of expected gnu binaries. As suggested by @Artturin in PR comment: <NixOS#140076 (comment)>.
Required move from libsForQt514 -> libsForQt515. Note that this changset won't be backportable to 21.05.
Done updating to latest in 975ab7f (intended only for unstable). Tested a complete session with another computer (launching |
to test the module https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules |
Could you squash all the commits other than the |
Yes, I know how to squash however, each of these changeset really are standalone changes that fix distinct unrelated issues atomically. Is that really advisable to squash and if so, do you have a ref to some contributing doc that describe this per PR squash requisite? What kind of name would you give this changeset so that it fits contributing guidelines? |
I agree that it would be a great idea to add some testing to this brittle module. Wouldn't it be better to introduce this module testing as a separate PR? It seems to be like quite a job to test this module properly and would delay this PR which fixes major issues. I pretty much exhausted the time I can spend on open source contributions at this time. |
nixpkgs has over 322000 commits and if we would do atomic commits for every little piece of code change then we would be way higher and git would be yet a bit slower. |
These changes are standalone. Everything is good. |
This centralizes `PATH` and `LD_LIBRARY_PATH`, avoid multiple layers of wrappers. Refactor as suggested by @Artturin in PR provided patch: <NixOS#140076 (comment)>.
Simply add `coreutils` as a runtime dependency which will prevent teamviewer from using incomplete busybox implementation of expected gnu binaries. As suggested by @Artturin in PR comment: <NixOS#140076 (comment)>.
@jagajaga: Thanks for the merge. |
A couple of changes suggested by @SuperSandro2000 that were not merged.
@SuperSandro2000 / @Artturin: Will be working on https://github.com/jraygauthier/nixpkgs/commits/jrg/post-140076-pr-teamviewer for change requests that did not make the merge and create a separate PR once I have some interesting module test ready. |
the wiki page shows how a user can test a module (which it seems that you did), not how to write automatic tests for it |
Ok, I assumed you meant automated testing and did not even click the link. Yes I was aware of the I do think it might ease maintenance if we had some level of automated testing to at least prevent the usual version upgrade without manual testing I observed in the past or the dependency update that crash us at runtime. I think something like this might do well:
|
This centralizes `PATH` and `LD_LIBRARY_PATH`, avoid multiple layers of wrappers. Refactor as suggested by @Artturin in PR provided patch: <NixOS#140076 (comment)>.
Simply add `coreutils` as a runtime dependency which will prevent teamviewer from using incomplete busybox implementation of expected gnu binaries. As suggested by @Artturin in PR comment: <NixOS#140076 (comment)>.
Successfully created backport PR #141439 for |
This centralizes `PATH` and `LD_LIBRARY_PATH`, avoid multiple layers of wrappers. Refactor as suggested by @Artturin in PR provided patch: <#140076 (comment)>. (cherry picked from commit c55bc5b)
Simply add `coreutils` as a runtime dependency which will prevent teamviewer from using incomplete busybox implementation of expected gnu binaries. As suggested by @Artturin in PR comment: <#140076 (comment)>. (cherry picked from commit 4fb188e)
…15.15.5 -> 15.18.5 -> 15.22.3 (#141439) * teamviewer: fix issue #96633 Fix teamviewer's breakage post 15.5.3 -> 15.15.5. Teamviewer client was no longer able to connect to its backing server as it now uses dbus to do so. Following changes were required: - add missing dbus and polkit service/policy files to package. - add missing dbus lib to `LD_LIBRARY_PATH`. Changes to the nixos module as a separate changeset. (cherry picked from commit 506966d) * nixos/teamviewer: fix issue #96633 Add teamviewer package as a dbus package now that the client / server communication depends on dbus. (cherry picked from commit 200e959) * nixos/teamviewer: fix issue #44307 Move to a forefront launch of the daemon. Doing so allowed us to move the service from forking to simple to avoid the missing pid error log. Also: - Make the dbus dependency explicit. (cherry picked from commit 953bbc0) * teamviewer: 15.15.5 -> 15.18.5 Upgrading to the last version still using qt5.14. Later version will be using qt5.15 which is not in 21.05 stable branch. This fixes us the crash observed in 15.15.5 when stopping the service. (cherry picked from commit db889eb) * teamviewer: refactor executable wrapping This centralizes `PATH` and `LD_LIBRARY_PATH`, avoid multiple layers of wrappers. Refactor as suggested by @Artturin in PR provided patch: <#140076 (comment)>. (cherry picked from commit c55bc5b) * teamviewer: fix 97148 (busybox installed issue) Simply add `coreutils` as a runtime dependency which will prevent teamviewer from using incomplete busybox implementation of expected gnu binaries. As suggested by @Artturin in PR comment: <#140076 (comment)>. (cherry picked from commit 4fb188e) * teamviewer: 15.18.5 -> 15.22.3 Required move from libsForQt514 -> libsForQt515. Note that this changset won't be backportable to 21.05. (cherry picked from commit 975ab7f) Co-authored-by: Raymond Gauthier <jraygauthier@gmail.com>
Motivation for this change
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)Tested the service with these 2 patches applied on top of my current 21.05 config. Works fine.
Tested client server launching these manually from package built from this branch. Works fine and fix the bug.
Note that
nixpkgs-review
raise an error which I am completely unable to understand:If you look at the package, it effectively sets
dontWrapQtApps = true;
.